Skip to content
This repository has been archived by the owner on Jun 6, 2023. It is now read-only.

Changes based on integrating with ADFS, Azure, and PingOne #7

Open
wants to merge 35 commits into
base: master
Choose a base branch
from

Conversation

samv
Copy link

@samv samv commented Jan 13, 2016

Just to let you know, we've been successfully integrating our product against various SAML implementations using this code, and here are some of the changes which we've found are necessary. There's a caveat that not all the changes are completely API compatible, and we're not done, but I just thought I'd open this PR so we can start talking about what it will take to converge.

samv and others added 30 commits November 18, 2015 17:51
Due to the way these elements were declared, building a request with no
signature still sends the empty tags:

   ...

To keep the request small and for better compatibility, suppress them
altogether with unsigned requests.
They were defining a prefix which was already defined in the scope of their
definition, and it seems encoding/xml isn't clever enough to know that it
doesn't need to emit them in that case.  Remove them.
This may or may not be required for our use case.  Slack sets this to
persistent in their implementation and I was changing things I could see.
Azure thinks dateTime values with nanosecond precision are not protocol
compliant.

http://www.w3.org/TR/xmlschema-2/#dateTime - Azure is wrong.

It's OK, there's an ID in there anyway to resolve ambiguity.
This value is required in order to implement logout; parse it.
Setting this field to the string "true" will force the identity provider
(with the power of MUST) to authenticate the presenter directly rather than
rely on a previous security context.
The type changed in order to support unsigned requests, but the
test did not update.
Callers may want to store the decoded XML, so allow them to do the
base64 decoding and pass in the XML string.  This is needed in order
to set the 'originalString' method for validation.
The XML Signature standard is designed to be re-usable standard by
other standards, and it's up to those other standards how they bind it
into their own messages.  In SAML, the binding allows signatures to
appear in a number of different places.  XML Signatures can't assume
anything about the document structure for the standard it is used
with, so instead it uses references to specify which part of the
document the signature applies to.  A given directory may be
consistent about which node it signs, and the previous implementation
was coded to assume the signature appears on the root element.
However, with Azure the "Assertion" block is the signed part.

Update the implementation to delegate responsibility of which node to
pass to "xmlsec" for signature verification to a response method, and
add a basic implementation which supports ID referencing.
Especially not with spaces.  Eww.  What a waste of space.
Some implementations might accept xml elements at a particular level
in any order, but due to the design of XMLSchema, typically only one
ordering is valid.  Azure rejects requests with the signature in the
wrong place.  Fix this and another minor compliance issue
(capitalization of an attribute not matching the spec)
Looks like I was mistaken; it's ID on the SAML specs, but Id on the
XML Signature spec.
ADFS fails with 'ID6027: Enveloped Signature Transform cannot be the
last transform in the chain.  The last transform must compute the
digest which Enveloped Signature transform is not capable of.'

The signature block in requests is now essentially identical to the
one returned by Azure and ADFS.
From [SAMLCore], §3.2.1:

    Destination [Optional]

    A URI reference indicating the address to which this request has
    been sent. This is useful to prevent malicious forwarding of
    requests to unintended recipients, a protection that is required
    by some protocol bindings. If it is present, the actual recipient
    MUST check that the URI reference identifies the location at which
    the message was received. If it does not, the request MUST be
    discarded. Some protocol bindings may require the use of this
    attribute (see [SAMLBind]).

In the SAML Bindings spec, §3.4.5.2 we find:

    Security Considerations

    The presence of the user agent intermediary means that the
    requester and responder cannot rely on the transport layer for
    end-end authentication, integrity and confidentiality. URL-encoded
    messages MAY be signed to provide origin authentication and
    integrity if the encoding method specifies a means for signing.
    If the message is signed, the Destination XML attribute in the
    root SAML element of the protocol message MUST contain the URL to
    which the sender has instructed the user agent to deliver the
    message. The recipient MUST then verify that the value matches the
    location at which the message has been received.

ie, this URL is here to prevent someone from taking a login request
for one IdP and using it for a different IdP.
These elements are not required by the standard, and some directory servers will
refuse requests with them set.  Change their types to be pointers so that they
can be omitted.
It's possible to encrypt SAML response assertions; this was primarily
useful in the era where it ran over non-encrypted endpoints, and is
still useful if there is some information in the assertions which you
don't want the user to be able to access using tools such as Firefox
SAML Tracer.  Being an OASIS standard, there are of course several
ways this can be arranged with relation to signatures: the assertions
can be signed and then encrypted, or the assertions can be encrypted
and the whole message signed.

This implementation allows for Assertion nodes to appear under
EncryptedAssertion, which is the form that 'xmlsec1' expects and
returns the plaintext versions, even though it doesn't conform to the
XML schema.  Callers should take utmost care to keep track of which
sections have been validated and which haven't when regarding
assertions as authorative.
These lines are a poor substitute for real exception logging, but carry it
on for now.
The code was still depending on the original library for utils etc; use the ones
in this fork instead.
Some directory servers require an explicit logout (especially if they don't
honor the 'ForceAuthn' flag on AuthnRequest).  Add support for this, including
signing.
Switching these URLs was necessary for using our branch, but now the branch
is being submitted for upstream inclusion, they should be returned to their
original fork.
SAML configuration information is exchanged in this format; add
support for reading and writing it.
…ure on response, assertion and encryptedassertion (#3)
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants